Skip to content

Debug wall collisions and particle state writing - #1745

Open
danieljvickers wants to merge 8 commits into
MFlowCode:masterfrom
danieljvickers:debug-wall-collisions
Open

Debug wall collisions and particle state writing#1745
danieljvickers wants to merge 8 commits into
MFlowCode:masterfrom
danieljvickers:debug-wall-collisions

Conversation

@danieljvickers

@danieljvickers danieljvickers commented Aug 20, 2026

Copy link
Copy Markdown
Member

Description

Branch created to debug some issues I was having with particles not colliding with one of the domain boundaries when slip wall was used. The solution wound up being that the variable populating slip wall BC at the top of the boundary was not populated on the GPU. Updates have been made.

While I was in the code, I also noticed that ib_state_wrt will not trigger in post-process is all IBs are from particle_clouds. This is because when it checks for the number of IBs to create the array, it was not including cloud particles, and therefore generated 0-lengthed arrays. I added the particle cloud count to this check, allowing it to store more IB information for output.

Type of change (delete unused ones)

  • Bug fix

Testing

I generated a 2D, 2000 particle case as:

import json
import math

# 2D shock wave interacting with a bed of 20 free-floating circular particles.

gam_a = 1.4

# Shock parameters (Mach 1.5)
mach_number = 1.5
pre_shock_pressure = 1
pre_shock_density = 1.4
pre_shock_speed = 0.0
post_shock_pressure = 2.4583
post_shock_density = 2.6069
post_shock_speed = 0.6944

domain_size = 4.0
wave_front = -1.5

total_time = 2.5
num_time_steps = 4000
dt = float(total_time / num_time_steps)
num_saves = 400
steps_to_save = int(num_time_steps / num_saves)

# Soft-sphere collision parameters (from 3D_mibm_sphere_head_on_collision)
collision_time = 20.0 * dt

# Particle bed parameters
bed_x = 0.25
bed_y = 0.0
bed_lx = 3.0
bed_ly = 3.92
particle_radius = 0.02
particle_mass = 0.2
particle_min_spacing = 0.005

print(
    json.dumps(
        {
            # Logistics
            "run_time_info": "T",
            # Computational Domain Parameters
            "x_domain%beg": -domain_size * 0.5,
            "x_domain%end": domain_size * 0.5,
            "y_domain%beg": -domain_size * 0.5,
            "y_domain%end": domain_size * 0.5,
            "cyl_coord": "F",
            "m": 1024,
            "n": 1024,
            "p": 0,
            "dt": dt,
            "t_step_start": 0,
            "t_step_stop": num_time_steps,
            "t_step_save": steps_to_save,
            # Simulation Algorithm Parameters
            "num_patches": 2,
            "model_eqns": 2,
            "alt_soundspeed": "F",
            "num_fluids": 1,
            "mpp_lim": "F",
            "mixture_err": "T",
            "time_stepper": 3,
            "weno_order": 5,
            "weno_eps": 1.0e-16,
            "weno_Re_flux": "T",
            "weno_avg": "T",
            "avg_state": 2,
            "mapped_weno": "T",
            "null_weights": "F",
            "mp_weno": "T",
            "riemann_solver": 2,
            "wave_speeds": 1,
            "fd_order": 2,
            "bc_x%beg": -17,
            "bc_x%end": -8,
            "bc_y%beg": -15,
            "bc_y%end": -15,
            # Immersed boundaries — all circles come from the particle bed
            "ib": "T",
            "num_ibs": 0,
            "viscous": "T",
            # Collision model (soft-sphere, from 3D_mibm_sphere_head_on_collision)
            "collision_model": 1,
            "coefficient_of_restitution": 0.9,
            "collision_time": collision_time,
            "ib_coefficient_of_friction": 0.1,
            # Particle bed: 20 free-floating circles placed randomly in region
            "num_particle_clouds": 1,
            "particle_cloud(1)%x_centroid": bed_x,
            "particle_cloud(1)%y_centroid": bed_y,
            "particle_cloud(1)%z_centroid": 0.0,
            "particle_cloud(1)%length_x": bed_lx,
            "particle_cloud(1)%length_y": bed_ly,
            "particle_cloud(1)%length_z": 0.0,
            "particle_cloud(1)%num_particles": 2000,
            "particle_cloud(1)%radius": particle_radius,
            "particle_cloud(1)%mass": particle_mass,
            "particle_cloud(1)%min_spacing": particle_min_spacing,
            "particle_cloud(1)%moving_ibm": 2,
            "particle_cloud(1)%seed": 42,
            "particle_cloud(1)%packing_method": 1,
            # Output
            "format": 1,
            "precision": 2,
            "prim_vars_wrt": "T",
            "E_wrt": "T",
            "ib_state_wrt": "T",
            "parallel_io": "T",
            # IC Patch 1: post-shock region (left of wave front)
            "patch_icpp(1)%geometry": 3,
            "patch_icpp(1)%x_centroid": 0.5 * wave_front - 0.25 * domain_size,
            "patch_icpp(1)%y_centroid": 0.0,
            "patch_icpp(1)%length_x": 0.5 * domain_size + wave_front,
            "patch_icpp(1)%length_y": domain_size,
            "patch_icpp(1)%vel(1)": post_shock_speed,
            "patch_icpp(1)%vel(2)": 0.0,
            "patch_icpp(1)%pres": post_shock_pressure,
            "patch_icpp(1)%alpha_rho(1)": post_shock_density,
            "patch_icpp(1)%alpha(1)": 1.0,
            # IC Patch 2: pre-shock region (right of wave front)
            "patch_icpp(2)%geometry": 3,
            "patch_icpp(2)%x_centroid": 0.5 * wave_front + 0.25 * domain_size,
            "patch_icpp(2)%y_centroid": 0.0,
            "patch_icpp(2)%length_x": 0.5 * domain_size - wave_front,
            "patch_icpp(2)%length_y": domain_size,
            "patch_icpp(2)%vel(1)": pre_shock_speed,
            "patch_icpp(2)%vel(2)": 0.0,
            "patch_icpp(2)%pres": pre_shock_pressure,
            "patch_icpp(2)%alpha_rho(1)": pre_shock_density,
            "patch_icpp(2)%alpha(1)": 1.0,
            # Fluid properties: air
            "fluid_pp(1)%gamma": 1.0 / (gam_a - 1.0),
            "fluid_pp(1)%pi_inf": 0,
            "fluid_pp(1)%Re(1)": 2500000,
        }
    )
)

This case was run on an RTX 2080 Super with NVHPC/23.11

Checklist

Check these like this [x] to indicate which of the below applies.

  • [] I added or updated tests for new behavior
  • I updated documentation if user-facing behavior changed

@codecov

codecov Bot commented Aug 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.71%. Comparing base (6c87019) to head (f620a4f).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1745      +/-   ##
==========================================
+ Coverage   61.67%   61.71%   +0.04%     
==========================================
  Files          84       84              
  Lines       21619    21642      +23     
  Branches     3196     3196              
==========================================
+ Hits        13334    13357      +23     
  Misses       6093     6093              
  Partials     2192     2192

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@danieljvickers

danieljvickers commented Aug 27, 2026

Copy link
Copy Markdown
Member Author

@sbryngelson I am pretty sure that this is not an issue with the branch, and I am beginning to have several branches all with scattered fixes to various things that I have been working on. Do we think that this is something to do with errors on the Phoenix runners, or have they been running successfully for others? Should I get on Phoenix and resolve this?

@sbryngelson

sbryngelson commented Aug 27, 2026

Copy link
Copy Markdown
Member

@sbryngelson I am pretty sure that this is not an issue with the branch, and I am beginning to have several branches all with scattered fixes to various things that I have been working on. Do we think that this is something to do with errors on the Phoenix runners, or have they been running successfully for others? Should I get on Phoenix and resolve this?

Thanks for letting me know. I am desperately trying to fix everything, as runner behavior changed again. Your code is probably fine.

@sbryngelson
sbryngelson self-requested a review August 27, 2026 14:19
@sbryngelson
sbryngelson marked this pull request as draft August 27, 2026 14:19
@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown

Lines of Code

File Lines Diff
src/post_process/m_global_parameters.fpp 376 +19
src/post_process/m_mpi_proxy.fpp 166 +10
Directory Lines Diff
post_process 3366 +29
total 45684 +29

@sbryngelson
sbryngelson marked this pull request as ready for review August 30, 2026 18:34
Copilot AI lite review requested due to automatic review settings August 30, 2026 18:34
Copilot AI reviewed Aug 30, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes GPU boundary-condition data synchronization for slip-wall collisions and corrects post-process IB state output when immersed boundaries are created exclusively from particle clouds.

Changes:

  • Sync ib_bc_*%end values to the GPU so slip-wall boundary handling is correct on all domain sides.
  • Broadcast/initialize particle-cloud metadata in post-process and include particle-cloud IB counts when sizing/reading IB-state outputs.
  • Update parameter definitions to classify num_particle_clouds / particle_cloud as pre-simulation inputs.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
toolchain/mfc/params/definitions.py Moves particle-cloud params into the pre-sim namelist set so they are recognized appropriately.
src/simulation/m_ibm.fpp Updates GPU data synchronization to include ib_bc_*%end boundary values.
src/simulation/m_global_parameters.fpp Declares GPU storage for ib_bc_*%end so the new GPU updates are valid.
src/simulation/m_collisions.fpp Removes a now-misleading comment around ghost-point overlap iteration.
src/post_process/m_mpi_proxy.fpp Adds MPI broadcast for particle_cloud(i)%... fields needed in post-process.
src/post_process/m_global_parameters.fpp Initializes particle-cloud defaults and adjusts num_ibs to include particle-cloud particles for IB-state reading.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/post_process/m_mpi_proxy.fpp
Comment on lines +98 to +100
#:for VAR in ['x_centroid', 'y_centroid', 'z_centroid', 'length_x', 'length_y', 'length_z', &
& 'radius', 'mass', 'min_spacing']
call MPI_BCAST(particle_cloud(i)%${VAR}$, 1, mpi_p, 0, MPI_COMM_WORLD, ierr)
Comment thread src/post_process/m_global_parameters.fpp
Comment on lines +342 to +347
! Particle clouds expand into individual IB patches at simulation startup, so num_ibs as read
! from the case file counts only the namelist patches. Match the global count the simulation
! arrives at (s_reduce_ib_patch_array) so the IB state records can be read back.
do i = 1, num_particle_clouds
num_ibs = num_ibs + particle_cloud(i)%num_particles
end do
@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown

Claude Code Review

Head SHA: f620a4f

Files changed:

  • 6
  • src/post_process/m_global_parameters.fpp
  • src/post_process/m_mpi_proxy.fpp
  • src/simulation/m_collisions.fpp
  • src/simulation/m_global_parameters.fpp
  • src/simulation/m_ibm.fpp
  • toolchain/mfc/params/definitions.py

Findings:

  • src/post_process/m_global_parameters.fpp (new default-init loop for particle_cloud, ~line 221-236): the loop sets x_centroid, y_centroid, z_centroid, length_x/y/z, num_particles, radius, mass, min_spacing, moving_ibm, seed, packing_method, but omits shell_inner_radius, shell_outer_radius, cloud_geometry, and periodic. The simulation target's equivalent loop (src/simulation/m_global_parameters.fpp, visible in the provided context) explicitly defaults all of these same four extra members. Since post_process never assigns them, they are read back as uninitialized memory on every rank (rank 0 never sets them either, so this is not just a broadcast gap) — the exact silent-uninitialized-struct-member failure pattern already called out in .claude/rules/common-pitfalls.md. If any post-process code path branches on cloud_geometry or the shell radii (plausible, since this PR adds logic specifically to let post_process "read back" particle-cloud-derived IB state), this will produce wrong or garbage results without a crash.
  • src/post_process/m_mpi_proxy.fpp (new particle_cloud broadcast loop, ~line 96-106): the manual broadcast list mirrors the incomplete default-init above — it also omits shell_inner_radius, shell_outer_radius, cloud_geometry, and periodic. Even if these fields were fixed to be defaulted on rank 0, non-root ranks would still not receive rank 0's case-file values for them, leaving per-rank state inconsistent for any post-process logic that depends on cloud geometry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@sbryngelson sbryngelson sbryngelson approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@danieljvickers @sbryngelson